fix(screenshots): Don't capture zero size screenshots by krystofwoldrich · Pull Request #2459 · getsentry/sentry-cocoa

krystofwoldrich · 2022-11-29T10:15:31Z

📜 Description

In Android SDK the screenshot function checks for invalid screenshots values, like zero-size screenshots. This was missing in iOS.

💡 Motivation and Context

feat: Screenshots sentry-react-native#2610 (comment)

💚 How did you test it?

I run the SDK locally with the RN sample app.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

github-actions · 2022-11-29T10:31:22Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1202.17 ms	1238.98 ms	36.81 ms
Size	20.75 KiB	383.57 KiB	362.81 KiB

Baseline results on branch: 8.0.0

Startup times

Revision	Plain	With Sentry	Diff
`a9e77dc`	1231.94 ms	1254.85 ms	22.91 ms
`8361c4c`	1204.07 ms	1252.74 ms	48.67 ms
`cdf9acd`	1219.62 ms	1254.80 ms	35.18 ms
`b9c9598`	1228.57 ms	1251.10 ms	22.53 ms
`034ff5e`	1231.22 ms	1255.37 ms	24.14 ms
`dd266f5`	1224.76 ms	1252.63 ms	27.87 ms
`2b42e4e`	1212.52 ms	1249.24 ms	36.72 ms
`bbf5334`	1216.84 ms	1238.82 ms	21.98 ms
`fb9f27a`	1218.55 ms	1249.17 ms	30.62 ms
`b9c9598`	1232.86 ms	1253.84 ms	20.98 ms

App size

Revision	Plain	With Sentry	Diff
`a9e77dc`	20.75 KiB	379.12 KiB	358.36 KiB
`8361c4c`	20.75 KiB	383.87 KiB	363.12 KiB
`cdf9acd`	20.75 KiB	383.78 KiB	363.03 KiB
`b9c9598`	20.75 KiB	383.77 KiB	363.01 KiB
`034ff5e`	20.75 KiB	383.83 KiB	363.07 KiB
`dd266f5`	20.75 KiB	383.39 KiB	362.64 KiB
`2b42e4e`	20.75 KiB	383.89 KiB	363.14 KiB
`bbf5334`	20.75 KiB	383.37 KiB	362.62 KiB
`fb9f27a`	20.75 KiB	382.11 KiB	361.36 KiB
`b9c9598`	20.75 KiB	383.77 KiB	363.01 KiB

Previous results on branch: fix-dont-capture-zero-size-screenshots

Startup times

Revision	Plain	With Sentry	Diff
`d91ca50`	1229.62 ms	1259.23 ms	29.61 ms
`d15071c`	1223.67 ms	1234.66 ms	10.99 ms
`63b24e1`	1261.49 ms	1272.02 ms	10.53 ms
`3c09ad9`	1194.24 ms	1228.64 ms	34.40 ms

App size

Revision	Plain	With Sentry	Diff
`d91ca50`	20.75 KiB	374.87 KiB	354.12 KiB
`d15071c`	20.75 KiB	374.63 KiB	353.88 KiB
`63b24e1`	20.75 KiB	374.87 KiB	354.12 KiB
`3c09ad9`	20.75 KiB	374.91 KiB	354.16 KiB

Sources/Sentry/SentryScreenshot.m

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>

philipphofmann · 2022-11-29T14:48:59Z

Sources/Sentry/SentryScreenshot.m

        if ([window drawViewHierarchyInRect:window.bounds afterScreenUpdates:false]) {
            UIImage *img = UIGraphicsGetImageFromCurrentImageContext();
-            [result addObject:UIImagePNGRepresentation(img)];
+            if (img.size.width > 0 || img.size.height > 0) {


m: Can we please add tests for this new functionality? As this code is not really testable you could extract this code into a method, expose it via a private category that you would have to add similar as we have it, for example, for SentryClient+Private.h, and then call it from tests. I can also push the basic solution to your branch if you want @krystofwoldrich.

You can push the solution to this branch and I will work with that. It will definitely help me.

Besides exposing it with the private header. Are there any examples in the test on how to mock functions, likely I will have to mock UIGraphicsGetImageFromCurrentImageContext.

The code was already testable. I added a test.

philipphofmann · 2022-11-29T15:21:29Z

@krystofwoldrich, does this need to be released quickly or can it wait until 8.0.0, which we are currently preparing?

krystofwoldrich · 2022-11-29T15:56:51Z

@philipphofmann It can wait.

philipphofmann

LGTM

philipphofmann · 2022-12-01T16:09:54Z

Sources/Sentry/SentryScreenshot.m

+                NSData *bytes = UIImagePNGRepresentation(img);
+                if (bytes && bytes.length > 0) {


l: If the image has 0 height or width, it's already empty. I think we don't need to check for bytes && bytes.length > 0 here.

Its not the size of the view that makes the image 0 length, its not being able to render the image.

philipphofmann · 2022-12-05T10:54:38Z

@krystofwoldrich, what's stopping us from merging this PR?

…o-size-screenshots

codecov · 2022-12-05T12:14:26Z

Codecov Report

Merging #2459 (8b99397) into 8.0.0 (c6fb5a9) will increase coverage by 88.41%.
The diff coverage is 100.00%.

❗ Current head 8b99397 differs from pull request most recent head 15331d3. Consider uploading reports for the commit 15331d3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           8.0.0    #2459       +/-   ##
==========================================
+ Coverage   0.00%   88.41%   +88.41%     
==========================================
  Files        239      178       -61     
  Lines      22236    15211     -7025     
  Branches    9134     5282     -3852     
==========================================
+ Hits           0    13449    +13449     
+ Misses     22231     1583    -20648     
- Partials       5      179      +174

Impacted Files	Coverage Δ
Sources/Sentry/SentryScreenshot.m	`69.56% <100.00%> (+69.56%)`	⬆️
Sources/Sentry/SentryUIApplication.m	`0.00% <0.00%> (ø)`
...ces/SentryCrash/Recording/SentryCrashReportStore.c
...ash/Recording/Monitors/SentryCrashMonitor_Zombie.c
...ding/Tools/SentryCrashStackCursor_MachineContext.h
...yCrash/Recording/Monitors/SentryCrashMonitorType.h
...es/SentryCrash/Recording/Tools/SentryCrashSysCtl.c
Sources/SentryCrash/Recording/Tools/SentryHook.c
...entryCrash/Recording/Monitors/SentryCrashMonitor.c
...Crash/Reporting/Filters/Tools/SentryCrashVarArgs.h
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6fb5a9...15331d3. Read the comment docs.

krystofwoldrich · 2022-12-05T12:15:17Z

@philipphofmann Nothing, just waiting for CI to finish and it's merged.

philipphofmann · 2022-12-05T17:58:16Z

Thanks @krystofwoldrich for the fix 👏😀

fix(screenshots): Don't capture zero size screenshots

ed15636

krystofwoldrich requested review from armcknight, brustolin and philipphofmann as code owners November 29, 2022 10:15

Update changelog with pr id

7d9c595

krystofwoldrich mentioned this pull request Nov 29, 2022

feat: Screenshots getsentry/sentry-react-native#2610

Merged

9 tasks

brustolin reviewed Nov 29, 2022

View reviewed changes

Sources/Sentry/SentryScreenshot.m Outdated Show resolved Hide resolved

Update Sources/Sentry/SentryScreenshot.m

d8f9d5f

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>

krystofwoldrich requested a review from brustolin November 29, 2022 12:21

brustolin and others added 2 commits November 29, 2022 11:00

Merge branch 'master' into fix-dont-capture-zero-size-screenshots

49f2f84

Format code

645d469

brustolin approved these changes Nov 29, 2022

View reviewed changes

philipphofmann reviewed Nov 29, 2022

View reviewed changes

Merge branch 'master' into fix-dont-capture-zero-size-screenshots

9a77f8a

philipphofmann changed the base branch from master to 8.0.0 November 29, 2022 16:08

philipphofmann added 3 commits December 1, 2022 16:58

Merge branch '8.0.0' into fix-dont-capture-zero-size-screenshots

6022f6a

Add test

c8a2689

fix changelog

8b99397

philipphofmann approved these changes Dec 1, 2022

View reviewed changes

philipphofmann reviewed Dec 1, 2022

View reviewed changes

Merge remote-tracking branch 'origin/8.0.0' into fix-dont-capture-zer…

15331d3

…o-size-screenshots

krystofwoldrich merged commit 323cdfe into 8.0.0 Dec 5, 2022

krystofwoldrich deleted the fix-dont-capture-zero-size-screenshots branch December 5, 2022 13:38

		NSData *bytes = UIImagePNGRepresentation(img);
		if (bytes && bytes.length > 0) {

Uh oh!

Conversation

krystofwoldrich commented Nov 29, 2022

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: 8.0.0

Startup times

App size

Previous results on branch: fix-dont-capture-zero-size-screenshots

Startup times

App size

Uh oh!

Uh oh!

philipphofmann Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

krystofwoldrich Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann commented Nov 29, 2022

Uh oh!

krystofwoldrich commented Nov 29, 2022

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

philipphofmann Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

brustolin Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann commented Dec 5, 2022

Uh oh!

codecov bot commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

krystofwoldrich commented Dec 5, 2022

Uh oh!

philipphofmann commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

github-actions bot commented Nov 29, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading